-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(build): enable imports directly from extensions and utils folder #77
Conversation
env: | ||
HUSKY: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to skip husky pre-commit hook since it causes an error when running semantic-release from the dist folder
|
||
- name: Run semantic release (release + bump version) | ||
run: npx semantic-release | ||
run: | | ||
cd dist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could tell semantic-release doesn't allow specifying a path to package.json, so instead we run it from the same folder as the package.json in dist
@@ -17,13 +17,8 @@ | |||
"bugs": { | |||
"url": "https://github.com/Faire/mjml-react/issues" | |||
}, | |||
"main": "dist/src/index.js", | |||
"files": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to specify files. We copy this to the dist folder, and we want to include everything in the dist folder in the publish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Should we write a migration path for this change in the readme?
src/index.tsx
Outdated
@@ -3,6 +3,8 @@ | |||
* Modify `scripts/generate-mjml-react.ts` to make changes to these files | |||
*/ | |||
|
|||
export * from "./utils"; | |||
|
|||
export { Mjml } from "./mjml/Mjml"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to create an src/mjml/index.tsx
and move all these generated exports from that folder?
src/index.tsx
could then be
export * from "./utils";
export * from "./mjml";
This would also simplify the AllComponents
in the jest configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I can implement that
const destination = `dist/${fileToCopy}`; | ||
if (fileToCopy === "package.json") { | ||
const file = fs.readFileSync(fileToCopy); | ||
fs.writeFileSync(destination, file.toString().replace(/dist\//g, "")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice and simple 🥂
🎉 This PR is included in version 3.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fix for #71.
Currently the package is built from the root directory, which causes imports from extensions or utils to have to have /dist/src in them. Instead we want to move the package.json to the dist folder (after the code is built) and publish that. WE also move the readme and the license with it.
Import before:
import { render } from "@faire/mjml-react/dist/src/utils/render";
Import after:
import { render } from "@faire/mjml-react/utils/render";
This has been tested on a beta version:
Published files in v3.0.1 (current latest)
Published files in beta version